Skip to content

Add opt-in --analyze-ghost-table-before-cutover - #1747

Open
VandhanaSelvaprakash-at wants to merge 2 commits into
github:masterfrom
VandhanaSelvaprakash-at:vaselvap-upstream-analyze-cutover
Open

Add opt-in --analyze-ghost-table-before-cutover#1747
VandhanaSelvaprakash-at wants to merge 2 commits into
github:masterfrom
VandhanaSelvaprakash-at:vaselvap-upstream-analyze-cutover

Conversation

@VandhanaSelvaprakash-at

Copy link
Copy Markdown

Supersedes #1419 (original approach and credit to @wangzihuacool) and closes the gap discussed in #1418. Thanks @ericyan / @timvaillancourt for the go-ahead.

What

Adds an opt-in --analyze-ghost-table-before-cutover flag (default off). When set, gh-ost runs an explicit ANALYZE TABLE on the ghost table immediately before cut-over, so the freshly swapped table doesn't briefly serve traffic with near-zero InnoDB row estimates — which the optimizer can cost as a free full scan, flipping plans on hot paths until statistics recompute.

Why opt-in

Per @shaohk and @timvaillancourt on #1419: on partitioned tables ANALYZE cost grows with partition count and the statement replicates. So it's gated behind a flag, default off, intended for small non-partitioned tables — opt-in users can report on performance.

How it differs from #1419

  1. Runs after the postpone gate releases (before the source lock and before --test-on-replica stops replication) — a postponed cut-over doesn't re-stale its statistics before the swap.
  2. A failed ANALYZE aborts the migration (fatal), fail-closed. ANALYZE TABLE surfaces table-level failures (missing table, storage-engine errors) as Msg_type=Error result rows while the statement succeeds at the protocol level, so the rows are inspected and cut-over is refused unless status is OK with no Error rows.

Tests

  • TestClassifyAnalyzeTableResult — DB-free table test of the result-row classifier (status-OK, case folding, error rows, full-scan ordering, non-OK status, empty result).
  • ApplierTestSuite.TestAnalyzeGhostTable — real-MySQL test covering the happy path, the fail-open regression, and the statement-error branch.

Also folds in a separable pre-existing fix: TeardownSuiteTearDownSuite across the applier/migrator/streamer suites (testify never invoked the misspelled method, leaking a testcontainer per suite). Happy to split it into its own PR if you'd prefer.

DCO signed off.

Add --analyze-ghost-table-before-cutover. When set, cutOver() runs an
explicit ANALYZE TABLE on the ghost table after the postpone gate releases
— before atomicCutOver() takes the source lock and before --test-on-replica
stops replication — logs the elapsed milliseconds on success, and aborts
the migration (fatal) if the ANALYZE fails, rather than swap in a table
with stale InnoDB statistics.

The abort exits synchronously (Log.Fatale), not via a retriable return — a
plain return re-runs cutOver() and the ANALYZE up to --default-retries.
Because ANALYZE TABLE reports table-level failures (missing table,
storage-engine errors) as Msg_type Error rows in its result set while
succeeding at the protocol level, the result rows are inspected and
cut-over is refused unless ANALYZE reports status OK with no Error rows;
privilege-style failures surface as statement errors on the same abort
path.

Without this, a freshly swapped table can briefly serve traffic with a
near-zero row estimate, which the optimizer may cost as a free full scan on
hot query paths, flipping plans until statistics are recomputed. Issue
github#1418 / PR github#1419 propose an ANALYZE for the same reason; this variant
corrects two defects there — the ANALYZE runs after the postpone gate (so a
postponed cut-over still gets fresh statistics) and a failed ANALYZE aborts
instead of being ignored. Opt-in, matching the maintainers' ask on github#1419
(ANALYZE cost grows with partition count, and the statement replicates).

The result-row inspection is extracted as classifyAnalyzeTableResult, a
pure, DB-free function, and covered by:
- TestClassifyAnalyzeTableResult: a table test over status-OK, case
  folding, an error row (alone and alongside a status-OK row), a status-OK
  row followed by a later error row (rows are scanned fully, not
  short-circuited), a non-OK status, and an empty result. Each refusal
  asserts the underlying cause via ErrorContains.
- ApplierTestSuite.TestAnalyzeGhostTable (real MySQL): happy path; the
  fail-open regression (dropping the ghost table makes ANALYZE return an
  Error row with no statement error, which the row inspection must refuse);
  and the statement-error branch (a closed connection is refused via the
  distinct error path).

Also fixes a pre-existing suite bug surfaced while adding the test above:
testify's suite runner calls TearDownSuite() (capital D), but the applier,
migrator, and streamer suites all spelled it TeardownSuite(), so the method
never matched the interface and the MySQL testcontainer was never
terminated. Renamed in all three suites.

Co-authored-by: wangzihuacool <wangzihuacool@163.com>
Signed-off-by: Vandhana Selvaprakash <vandhana.selvaprakash@airtable.com>

@ericyan ericyan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @VandhanaSelvaprakash-at, thanks for the PR! This looks great overall. Just need a tiny change to make the linter happy.

Could we add a small, deterministic Migrator-level test for this as well?

The applier tests cover ANALYZE execution and result parsing well, but they do not verify the cut-over orchestration contract: that the flag gates the call, it runs only after postpone releases, and an analysis failure stops before replica-stop or cut-over locking/retry work begins. A narrow test seam around the analysis invocation is fine; no need to abstract the full Applier just for this.

Adding localtest for this case would also be nice, but that is not required.

Comment thread go/logic/applier.go Outdated
- applier: name the AnalyzeGhostTable receiver `apl`, consistent with the
  rest of applier.go (staticcheck ST1016) — fixes the golangci-lint failure.
- migrator: extract the pre-cut-over ANALYZE gating into
  analyzeGhostTableBeforeCutOver(analyze func() error), a narrow injectable
  seam (behavior unchanged), so the orchestration contract is unit-testable
  without a live applier or the process-exiting Log.Fatale path.
- migrator test: TestAnalyzeGhostTableBeforeCutOver covers the flag gating,
  the happy path (ANALYZE runs once), and fail-closed (a failed ANALYZE
  propagates so cut-over aborts before replica-stop / cut-over locking).
- localtest: analyze-ghost-table-before-cutover exercises the flag end-to-end
  against live MySQL with ongoing DML.

Signed-off-by: Vandhana Selvaprakash <vandhana.selvaprakash@airtable.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants